-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DataGrid] Use event.defaultMuiPrevented to prevent the default behavior #2179
Conversation
01ce81c
to
bed8472
Compare
66c07f1
to
f361c79
Compare
f361c79
to
d2030d0
Compare
@@ -206,139 +206,166 @@ export interface GridOptions { | |||
/** | |||
* Callback fired when the edit cell value changes. | |||
* @param {GridEditCellPropsParams} params With all properties from [[GridEditCellPropsParams]]. | |||
* @param {React.SyntheticEvent} event The event that caused this prop to be called. | |||
* @param {MuiEvent} event The event that caused this prop to be called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param {MuiEvent} event The event that caused this prop to be called. | |
* @param {MuiEvent<React.SyntheticEvent>} event The event that caused this prop to be called. |
? x the other
I would personally be fine if I had to do this in my codebase.:
(event as any).defaultMuiPrevented = true;
* @param {MuiEvent} event The event that caused this prop to be called. | |
* @param {React.SyntheticEvent} event The event that caused this prop to be called. |
but I can understand if we don't like this idea 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We started to use this MuiEvent
. Would be nice to keep it, because of the special property. Sometimes using it can lead to terrible typings when the event can be fired both by a synthetic event as well as by a native event, e.g. MuiEvent<React.MouseEvent | DocumentEventMap['click']>
.
@@ -8,7 +8,7 @@ title: Data Grid - Events | |||
|
|||
## Subscribing to events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This page is not good. My idea is to remove the Pro badge from the page and move it to this section only. Then, add a new section only for the event props, e.g. onCellClick
, explaining that they are like subscribing to events but with less granularity. For the catalog of events, I want to improve the docs generator to include a third column with the event prop that is mapped to each event name. I think that with our convention GRID_CELL_CLICK -> onCellClick I can discover the mappings. cc @oliviertassinari
params: any, | ||
event: MuiEvent<React.SyntheticEvent | DocumentEventMap[keyof DocumentEventMap] | {}> = {}, | ||
) => { | ||
event.defaultMuiPrevented = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we set event.defaultMuiPrevented = true;
and then publish then it will be reset here and be published.
Why not check defaultMuiPrevented
everywhere there isPropagationStopped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still want the event to be published. Only the listeners should not run if event.defaultMuiPrevented
is true
.
@@ -12,7 +13,15 @@ export function useGridApiEventHandler( | |||
|
|||
React.useEffect(() => { | |||
if (handler && eventName) { | |||
return apiRef.current.subscribeEvent(eventName, handler, options); | |||
const enhancedHandler = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that could go out of this scope in a separate function. Not sure about redefining the handler as it changes the ref of the handler func potentially breaking immutability if (handler === otherHandler)
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any place where we compare the handler that is actually passed to subscribeEvent
. I mean, compare the handler with the one that is saved in the event emitter. The ref would not change.
Another approach would be to move the event.defaultMuiPrevented
validation to the event emitter, which is a very low level way to do it.
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Breaking changes
[DataGrid] Replace
event.stopPropagation()
withevent.defaultMuiPrevented = true
in all callback props.Previously, calling
event.stopPropagation
would stop the propagation of an event and also prevent its the default behavior from running. Now, these two actions can be controlled independently:Set
event.defaultMuiPrevented
totrue
to prevent the default action of an eventCall
event.stopPropagation
to stop an event from propagating to parent elements[XGrid] Replace
event.stopPropagation()
withevent.defaultMuiPrevented = true
in all event listeners.For more information, check this page.
Closes #1403
Preview: https://deploy-preview-2179--material-ui-x.netlify.app/components/data-grid/events/#disabling-the-default-behavior
TODO
event.isPropagationStopped()
MuiEvent